Adding `lib` support to crossgen #12

spacekookie · 2018-09-13T16:07:54Z

Choose one: This a 🙋 feature

So, this is just quickly something I threw together on the plane – looking for
some feedback!

The CLI now has a --lib flag which is taken into account when creating
a template. Template::new still exists, but is deprecated and points
to Template::gen_bin (just in case this shouldn't be a breaking change).

Actually generating the templates I'm not 100% sure what differences
might be wanted/required for bin vs lib. So I just copied the templates
directory for now.

Also the write block is quite ugly now. Considering that this approach
generally isn't very modular, I'm wondering if it wouldn't be possible
to have a folder with travis "snippets" that are then assembled according
to some user config (e.g. I want android but not wasm, but it's also a binary?)

Again...WIP – happy to change stuff!

Cheers!

Checklist

tests pass
tests and/or benchmarks are included
documentation is changed or added

Context

This PR addresses issue #11

Semver Changes

This is a non-breaking change (for now)

bltavares · 2018-09-14T00:56:54Z

templates/lib/before_deploy.ps1

+
+$ZIP = "$SRC_DIR\$($Env:CRATE_NAME)-$($Env:APPVEYOR_REPO_TAG_NAME)-$($Env:TARGET).zip"
+
+Copy-Item "$SRC_DIR\target\$($Env:TARGET)\release\$PKG_NAME.exe" '.\'


On windows we might want to have this as $PKG_NAME.dll and copy it to .\win-$target-$PKG_NAME.dll as I think GitHub don't support folders

bltavares · 2018-09-14T00:58:59Z

templates/lib/before_deploy.ps1

+New-Item -Type Directory -Name $STAGE
+Set-Location $STAGE
+
+$ZIP = "$SRC_DIR\$($Env:CRATE_NAME)-$($Env:APPVEYOR_REPO_TAG_NAME)-$($Env:TARGET).zip"


When thinking of the distribution aspect of the lib on github, I think it would be more helpful to have the bare-ddl not zipped.

In my mind, when building a library on another language, it could be possible to instrument the build system to download all the .so and .dll into a folder to distribute the package with it, and having it not-zipped would make the build system much simpler.

What do you think?

I guess the zip archive really only makes a big difference if you have a library that produces more than one artefact (which can easily be the case). Maybe it should be something that is configurable somehow?

bltavares · 2018-09-14T01:00:45Z

templates/lib/before_deploy.sh

+  test -f Cargo.lock || cargo generate-lockfile
+
+  cross rustc --bin $PKG_NAME --target $TARGET --release -- -C lto
+  cp target/$TARGET/release/$PKG_NAME $stage/


I think cargo generates libs in the form of ${PKG_NAME}lib.so

I thought it was lib${PKG_NAME}.so unless you manually set the lib.name field?

Oh, you are right!

bltavares · 2018-09-14T01:02:30Z

templates/lib/install.sh

+
+  # Install test dependencies
+  rustup component add rustfmt-preview
+  rustup component add clippy-preview


What if we also install bindgen and setup a build stage responsible to build the .h headers and publish on Github?

A few quick notes here:

we would need cbindgen, not bindgen here (e.g. wrap rust code to be consumed from C, not the other way around)

here's an example of how to do that

in the example it stands out to me that a cbindgen.toml file is included. Should we generate that as well, or leave that up to the user? It looks like it might not be required.

building with cbindgen can either be done from the command line or from build.rs, I'm not sure which one is preferable. But we should probably pick one.

bltavares · 2018-09-14T01:07:49Z

That is neat! I would be able to have some time next week to test drive this on a sample project.

One thing I've considered when reading the Issue would be to provide some feedback on the Cargo.toml setup, or even modify it automatically to add the crate-type = ["rlib", "dylib", ...] if not there yet. This could come later as well, but it would be a nice touch to me.

yoshuawuyts · 2018-09-14T10:36:13Z

src/cli.rs

@@ -18,6 +18,9 @@ pub struct Cli {
  /// Project name. Defaults to target directory name
  #[structopt(short = "n", long = "name")]
  name: Option<String>,
+  /// Specify if a library template should be generated
+  #[structopt(short = "l", long = "lib")]
+  lib: Option<bool>,


I believe lib: bool should work well enough. It defaults to false, and is only set to true if you pass --lib to the executable. This should make it slightly easier to access in the code.

yoshuawuyts · 2018-09-14T10:37:06Z

src/main.rs

@@ -25,6 +25,7 @@ fn main() -> Result<(), ExitFailure> {

  let dir = args.dir()?;
  let name = args.name()?;
+  let lib = args.lib().unwrap_or(false);


If args.lib() returns a bool, we can remove .unwrap_or(false) from this line.

I opted for this so that it's more clear in main.rs what the default vaulue is (instead of hiding it in cli.rs. We can of course change this

Ah okay, gotcha. Perhaps a better way to be explicit about the default state is to add default_value=false to the structopt attribute. I was a bit confused why we had 3 states for a boolean value (e.g. not passed at all, passed as true, passed as false?) instead of defaulting to a "is this option enabled? yes / no"

yoshuawuyts

I like the general idea a lot; think this would be fine to merge as-is. Commented with a few small questions / quality-of-life improvements. Hope they're alright. Thanks for doing this! 🎉

bltavares · 2018-09-14T12:23:43Z

Oh, I've just remembered that you can pass to cargo which type of lib it should build. I guess that my idea to fiddle with Cargo.toml would not be necessary at all.

crossgen --lib --lib-type=dynamic could introduce a new interpolated variable on the build when calling cargo. The nice thing is that it would be easy to switch to a static lib by regenerating, or by changing the definitions in the build matrix.

spacekookie · 2018-09-14T15:12:42Z

cc/ @yoshuawuyts So I changed the CLI handling to be more direct (no Option<bool>).

I think the question of how the library template files should work is not an easy one to solve. And also, maybe we can come up with a better and more modular way of storing/ generating them – aka just having a few code snippets that can be assembled into larger scripts that get generated.

But that's a bit beyond what this PR was meant to be.

Also: not sure why the CI fails? Any advice on that?

yoshuawuyts · 2018-09-14T16:31:12Z

@spacekookie it looks like cargo fmt wasn't run, which is the root cause of the issues. Also DCO bot is failing because commits aren't signed off with git commit -s (but also strongly considering removing DCO bot for my own work btw).

Screenshot

This is the cargo fmt diff:

yoshuawuyts · 2018-09-14T16:32:25Z

I think the question of how the library template files should work is not an easy one to solve. And also, maybe we can come up with a better and more modular way of storing/ generating them – aka just having a few code snippets that can be assembled into larger scripts that get generated.

But that's a bit beyond what this PR was meant to be.

I agree that it'd be great to figure out something for this, but agree it's best to do so somewhere separate. Perhaps we could draft an issue for this where we can discuss the options?

spacekookie · 2018-09-15T08:29:55Z

@yoshuawuyts I'll try re-running rustfmt then, although I thought I had already done that. Is there some specific version I need to use? Also…I sign my commits with gpg but don't really use -s normally. I guess I'll just do that for now.

Also, I just opened #13 to discuss some of these issues 🎉

So, this is just quickly something I threw together on the plane – looking for some feedback! The CLI now has a `--lib` flag which is taken into account when creating a template. `Template::new` still exists, but is deprecated and points to `Template::gen_bin` (just in case this shouldn't be a breaking change). Actually generating the templates I'm not 100% sure what differences might be wanted/required for bin vs lib. So I just copied the templates directory for now. Also the write block is quite ugly now. Considering that this approach generally isn't very modular, I'm wondering if it wouldn't be possible to have a folder with travis "snippets" that are then assembled according to some user config (e.g. I want android but not wasm, but it's also a binary?) Again...WIP – happy to change stuff! Cheers! Signed-off-by: Katharina Fey <[email protected]>

Signed-off-by: Katharina Fey <[email protected]>

yoshuawuyts · 2018-09-17T10:52:49Z

Got a build to pass on a re-run (which means travis failures are just a clippy problem) https://travis-ci.com/yoshuawuyts/crossgen/jobs/145930817, so think this is good to merge for now! ✨

yoshuawuyts · 2018-09-19T08:52:42Z

v0.5.0 🎉

bltavares reviewed Sep 14, 2018

View reviewed changes

yoshuawuyts reviewed Sep 14, 2018

View reviewed changes

yoshuawuyts approved these changes Sep 14, 2018

View reviewed changes

spacekookie changed the title ~~[WIP] Adding lib support to crossgen~~ Adding lib support to crossgen Sep 14, 2018

spacekookie added 3 commits September 15, 2018 12:24

Making the lib flag a bool, removing the explicit default

8899563

Signed-off-by: Katharina Fey <[email protected]>

Running rustfmt for the PR

f6433af

Signed-off-by: Katharina Fey <[email protected]>

spacekookie force-pushed the master branch from dcabc2b to f6433af Compare September 15, 2018 11:25

yoshuawuyts merged commit 6879674 into yoshuawuyts:master Sep 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding `lib` support to crossgen #12

Adding `lib` support to crossgen #12

spacekookie commented Sep 13, 2018 •

edited

Loading

bltavares Sep 14, 2018

bltavares Sep 14, 2018

spacekookie Sep 14, 2018

bltavares Sep 14, 2018

spacekookie Sep 14, 2018

bltavares Sep 14, 2018

bltavares Sep 14, 2018

yoshuawuyts Sep 14, 2018 •

edited

Loading

bltavares commented Sep 14, 2018

yoshuawuyts Sep 14, 2018

yoshuawuyts Sep 14, 2018

spacekookie Sep 14, 2018

yoshuawuyts Sep 14, 2018

yoshuawuyts left a comment

bltavares commented Sep 14, 2018

spacekookie commented Sep 14, 2018 •

edited

Loading

yoshuawuyts commented Sep 14, 2018 •

edited

Loading

yoshuawuyts commented Sep 14, 2018

spacekookie commented Sep 15, 2018 •

edited

Loading

yoshuawuyts commented Sep 17, 2018

yoshuawuyts commented Sep 19, 2018


		$ZIP = "$SRC_DIR\$($Env:CRATE_NAME)-$($Env:APPVEYOR_REPO_TAG_NAME)-$($Env:TARGET).zip"

		Copy-Item "$SRC_DIR\target\$($Env:TARGET)\release\$PKG_NAME.exe" '.\'

Adding lib support to crossgen #12

Adding lib support to crossgen #12

Conversation

spacekookie commented Sep 13, 2018 • edited Loading

Checklist

Context

Semver Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

bltavares commented Sep 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoshuawuyts left a comment

Choose a reason for hiding this comment

bltavares commented Sep 14, 2018

spacekookie commented Sep 14, 2018 • edited Loading

yoshuawuyts commented Sep 14, 2018 • edited Loading

Screenshot

yoshuawuyts commented Sep 14, 2018

spacekookie commented Sep 15, 2018 • edited Loading

yoshuawuyts commented Sep 17, 2018

yoshuawuyts commented Sep 19, 2018

Adding `lib` support to crossgen #12

Adding `lib` support to crossgen #12

spacekookie commented Sep 13, 2018 •

edited

Loading

yoshuawuyts Sep 14, 2018 •

edited

Loading

spacekookie commented Sep 14, 2018 •

edited

Loading

yoshuawuyts commented Sep 14, 2018 •

edited

Loading

spacekookie commented Sep 15, 2018 •

edited

Loading